MWPW-161625: fix mas:pending event on requestUpdate #3388

3ch023 · 2024-12-17T19:14:10Z

when requestUpdate on MAS inline-price or checkout-link is called the 'mas:resolved' event is dispatched twice.
this PR is fixing first event to be 'mas:pending' and it is followed by a single 'mas:resolved' or 'mas:failed'

added nala tests to cover this too.

Test URLs:

aem-code-sync · 2024-12-17T19:14:13Z

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

Re-run PSI checks
Re-sync branch

Commits

a2148d7 ✅ (latest)

aem-code-sync · 2024-12-17T19:14:33Z

	Page	Scores	Audits	Google
📱	/?martech=off
🖥️	/?martech=off

github-actions · 2024-12-17T19:14:41Z

libs/deps/mas/commerce.js

⚠️ [eslint] _{reported by reviewdog 🐶}
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.

github-actions · 2024-12-17T19:14:42Z

libs/deps/mas/mas.js

⚠️ [eslint] _{reported by reviewdog 🐶}
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.

github-actions · 2024-12-17T19:14:42Z

libs/features/mas/dist/mas.js

⚠️ [eslint] _{reported by reviewdog 🐶}
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.

libs/features/mas/src/mas-element.js

codecov · 2024-12-17T19:16:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.46%. Comparing base (6eb0401) to head (a2148d7).
Report is 3 commits behind head on stage.

Additional details and impacted files

@@            Coverage Diff             @@
##            stage    #3388      +/-   ##
==========================================
- Coverage   96.47%   96.46%   -0.02%     
==========================================
  Files         258      258              
  Lines       60150    60135      -15     
==========================================
- Hits        58029    58007      -22     
- Misses       2121     2128       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yesil · 2024-12-18T07:59:29Z

nala/features/mas/docs/docs.test.js

This is nice 👍

yesil · 2024-12-18T07:59:58Z

nala/features/mas/docs/docs.spec.js

+      name: '@MAS-DOCS-checkout-link',
+      path: '/libs/features/mas/docs/checkout-link.html',
+      browserParams: '?theme=dark',
+      tags: '@mas-docs @checkout-link @commerce @smoke @regression @milo',


do we need mas- prefix for the tags?

i will ask @afmicka
i have a feeling that these tags should be unique for whole milo repo, so if we want an ability to run only our mas docs tests - then it should be @mas-docs, if we want to run 'all' docs - @docs
i am not sure which it should be

yes, those tags are global, and docs would be ambiguous

yesil · 2024-12-18T08:01:12Z

libs/features/mas/src/mas-element.js

@@ -153,6 +153,8 @@ export class MasElement {
        this.state = STATE_FAILED;
        this.update();
        this.log?.error('Failed:', { element: this.wrapperElement, error });
+        // Allow calling code to perform sync updates of this element
+        // before notifying observers about state change


actually, setImmediate is deprecated, should we try it with setTimeout(callback, 0) by this occasion?

i refactored it to just call notify function where we need to call it.. imo this thread breaks are making comprehension harder

yesil · 2024-12-18T08:05:43Z

libs/features/mas/src/mas-element.js

@@ -166,7 +168,8 @@ export class MasElement {
        if (options) this.options = options;


I wonder if you can listen the very first mas:pending event.
If you try to register an even listener for a <span is="price" data-wcs-osi="xyz">/span>, it is possible that, the pending event will be dispatched before you register.
To check. I would really remove pending all together, and consider the default state as pending.

removed it, agree it's an unnessary event

i would somewhere here though add some debug log to mention that element is pending resolution

github-actions · 2024-12-18T21:39:03Z

libs/deps/mas/merch-card-collection.js

⚠️ [eslint] _{reported by reviewdog 🐶}
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.

github-actions · 2024-12-18T21:39:04Z

libs/deps/mas/merch-sidenav.js

⚠️ [eslint] _{reported by reviewdog 🐶}
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.

github-actions · 2024-12-18T21:39:05Z

libs/deps/mas/merch-subscription-panel.js

⚠️ [eslint] _{reported by reviewdog 🐶}
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.

github-actions · 2024-12-18T21:39:07Z

libs/features/mas/src/constants.js

⚠️ [eslint] _{reported by reviewdog 🐶}
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.

github-actions · 2024-12-19T02:23:34Z

This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR.

yesil · 2024-12-19T09:03:51Z

libs/features/mas/src/inline-price.js

            selectOffers(await promise, options),
            options,
            version,
        );
+        this.masElement.notify();


this logic is handled at mas-element level(*), the custom elements (we may have more in the future) should not know how / when to notify.

(*) https://github.com/adobecom/milo/pull/3388/files#diff-b76179f008b9c485066e722d9e2b9265ccdbfe9d5abb33507d5ab8424e7a45b5L207-L219

got it, i moved it back into mas-element

yesil · 2024-12-19T09:04:09Z

libs/features/mas/src/checkout-link.js

@@ -162,13 +162,15 @@ export class CheckoutLink extends HTMLAnchorElement {
            { ...extraOptions, ...options },
            this,
        );
-        return this.renderOffers(
+        const result = this.renderOffers(


same comment as for inline-price

npeltier · 2024-12-18T08:59:20Z

libs/features/mas/src/mas-element.js

@@ -166,7 +168,8 @@ export class MasElement {
        if (options) this.options = options;
        this.state = STATE_PENDING;
        this.update();
-        setImmediate(() => this.notify());
+        // immediately notify observers about state change
+        this.notify();


couldn't we even remove notification for pending state?

did that, thx

npeltier · 2024-12-19T09:45:56Z

libs/features/mas/src/mas-element.js

@@ -166,7 +168,8 @@ export class MasElement {
        if (options) this.options = options;


i would somewhere here though add some debug log to mention that element is pending resolution

npeltier · 2024-12-19T09:46:40Z

nala/features/mas/docs/docs.spec.js

+      name: '@MAS-DOCS-checkout-link',
+      path: '/libs/features/mas/docs/checkout-link.html',
+      browserParams: '?theme=dark',
+      tags: '@mas-docs @checkout-link @commerce @smoke @regression @milo',


yes, those tags are global, and docs would be ambiguous

yesil · 2024-12-19T12:00:52Z

libs/features/mas/src/mas-element.js

@@ -148,6 +148,7 @@ export class MasElement {
        if (options) this.options = options;
        this.state = STATE_PENDING;
        this.update();
+        this.log?.debug('Pending:', { element: this.wrapperElement });


I wonder if this has any performance impact, this it needs to kind of serialize the wrapperElement.

yesil

nala tests failing?

github-actions · 2024-12-20T01:37:35Z

Reminder to set the Ready for Stage label - to queue this to get merged to stage & production.

Roycethan · 2024-12-20T01:49:30Z

Verified, Regressions covered

milo-pr-merge · 2025-01-02T09:16:42Z

Skipped 3388: "MWPW-161625: fix mas:pending event on requestUpdate" due to file "libs/deps/mas/commerce.js" overlap. Merging will be attempted in the next batch

milo-pr-merge · 2025-01-07T09:17:23Z

Skipped 3388: "MWPW-161625: fix mas:pending event on requestUpdate" due to file "libs/deps/mas/mas.js" overlap. Merging will be attempted in the next batch

Roycethan · 2025-01-07T16:38:00Z

@3ch023 Plz resolve conflicts in this PR to merge, thanks

github-actions · 2025-01-09T12:25:10Z

libs/deps/mas/merch-card.js

⚠️ [eslint] _{reported by reviewdog 🐶}
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.

milo-pr-merge · 2025-01-13T08:23:45Z

Skipped 3388: "MWPW-161625: fix mas:pending event on requestUpdate" due to file "libs/deps/mas/commerce.js" overlap. Merging will be attempted in the next batch

milo-pr-merge · 2025-01-14T09:15:41Z

Error merging 3388: MWPW-161625: fix mas:pending event on requestUpdate Pull Request is not mergeable

mokimo · 2025-01-14T09:31:14Z

@3ch023 seems like the merge of yesterday, lead to conflicts today

This branch has conflicts that must be resolved
libs/deps/mas/commerce.js
libs/deps/mas/mas.js
libs/deps/mas/merch-card.js
libs/features/mas/dist/mas.js

milo-pr-merge · 2025-01-14T12:19:23Z

Skipped 3388: "MWPW-161625: fix mas:pending event on requestUpdate" due to file "libs/deps/mas/merch-card-collection.js" overlap. Merging will be attempted in the next batch

3ch023 · 2025-01-14T13:32:15Z

@3ch023 seems like the merge of yesterday, lead to conflicts today

This branch has conflicts that must be resolved
libs/deps/mas/commerce.js
libs/deps/mas/mas.js
libs/deps/mas/merch-card.js
libs/features/mas/dist/mas.js

I want to automate this
https://github.com/orgs/adobecom/discussions/3443

milo-pr-merge · 2025-01-14T16:12:22Z

Skipped merging 3388: MWPW-161625: fix mas:pending event on requestUpdate due to failing checks

mokimo · 2025-01-14T16:24:51Z

@3ch023 you might need to rebase to stage inorder for the nala tests to run through. Not sure if its the same tests @afmicka told me to rebase for though 😁

milo-pr-merge · 2025-01-15T16:12:22Z

Skipped merging 3388: MWPW-161625: fix mas:pending event on requestUpdate due to failing checks

milo-pr-merge · 2025-01-16T16:17:31Z

Skipped merging 3388: MWPW-161625: fix mas:pending event on requestUpdate due to failing checks

3ch023 added the commerce label Dec 17, 2024

github-actions bot reviewed Dec 17, 2024

View reviewed changes

libs/features/mas/src/mas-element.js Show resolved Hide resolved

yesil reviewed Dec 18, 2024

View reviewed changes

aem-code-sync bot temporarily deployed to MWPW-161625 December 18, 2024 17:45 Inactive

github-actions bot reviewed Dec 18, 2024

View reviewed changes

yesil reviewed Dec 19, 2024

View reviewed changes

npeltier approved these changes Dec 19, 2024

View reviewed changes

aem-code-sync bot temporarily deployed to MWPW-161625 December 19, 2024 11:19 Inactive

yesil reviewed Dec 19, 2024

View reviewed changes

yesil approved these changes Dec 19, 2024

View reviewed changes

Roycethan added verified PR has been E2E tested by a reviewer Ready for Stage labels Dec 20, 2024

github-actions bot reviewed Jan 9, 2025

View reviewed changes

adobecom deleted a comment from milo-pr-merge bot Jan 10, 2025

resolve conflicts with refactored code

a2148d7

3ch023 force-pushed the MWPW-161625 branch from 8e18736 to a2148d7 Compare January 17, 2025 10:04

milo-pr-merge bot merged commit 965a8c6 into stage Jan 20, 2025
14 checks passed

milo-pr-merge bot deleted the MWPW-161625 branch January 20, 2025 09:25

milo-pr-merge bot mentioned this pull request Jan 20, 2025

[Release] Stage to Main #3512

Merged

		@@ -166,7 +168,8 @@ export class MasElement {
		if (options) this.options = options;

MWPW-161625: fix mas:pending event on requestUpdate #3388

MWPW-161625: fix mas:pending event on requestUpdate #3388

Conversation

3ch023 commented Dec 17, 2024

aem-code-sync bot commented Dec 17, 2024 • edited Loading

aem-code-sync bot commented Dec 17, 2024 • edited Loading

github-actions bot Dec 17, 2024

Choose a reason for hiding this comment

github-actions bot Dec 17, 2024

Choose a reason for hiding this comment

github-actions bot Dec 17, 2024

Choose a reason for hiding this comment

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3ch023 Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Dec 18, 2024

Choose a reason for hiding this comment

github-actions bot Dec 18, 2024

Choose a reason for hiding this comment

github-actions bot Dec 18, 2024

Choose a reason for hiding this comment

github-actions bot Dec 18, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 19, 2024

Choose a reason for hiding this comment

3ch023 Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yesil left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 20, 2024

Roycethan commented Dec 20, 2024

milo-pr-merge bot commented Jan 2, 2025

milo-pr-merge bot commented Jan 7, 2025

Roycethan commented Jan 7, 2025

github-actions bot Jan 9, 2025

Choose a reason for hiding this comment

milo-pr-merge bot commented Jan 13, 2025

milo-pr-merge bot commented Jan 14, 2025

mokimo commented Jan 14, 2025

milo-pr-merge bot commented Jan 14, 2025

3ch023 commented Jan 14, 2025

milo-pr-merge bot commented Jan 14, 2025

mokimo commented Jan 14, 2025

milo-pr-merge bot commented Jan 15, 2025

milo-pr-merge bot commented Jan 16, 2025

aem-code-sync bot commented Dec 17, 2024 •

edited

Loading

aem-code-sync bot commented Dec 17, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

3ch023 Dec 18, 2024 •

edited

Loading

3ch023 Dec 19, 2024 •

edited

Loading